Persist original ARM API version for use by generic controller #1606

theunrepentantgeek · 2021-06-26T01:43:20Z

What this PR does / why we need it:

Adds OriginalVersion() function to API Spec types to return the actual ARM API version requested by users.
Adds OriginalVersion property to Storage Spec types to store that version for later use.
Adds OriginalGVK() function to Storage Resource types to return the GVK of the original version.
Removes StorageTypeFactory in favour of separate pipeline sections

Also includes

Unit tests for OriginalVersion() function generation
Unit tests for OriginalGVK() function generation
Fix bug in prerequisite checks for pipeline stages
Fix bug in ErroredType where WriteDebugDescription() would crash if there was no wrapped type

How does this PR make you feel:

If applicable:

this PR contains tests

codecov-commenter · 2021-06-27T00:23:14Z

Codecov Report

Merging #1606 (8ebde28) into master (1e458b2) will increase coverage by 1.89%.
The diff coverage is 80.25%.

❗ Current head 8ebde28 differs from pull request most recent head 393d2a7. Consider uploading reports for the commit 393d2a7 to get more accurate results

@@            Coverage Diff             @@
##           master    #1606      +/-   ##
==========================================
+ Coverage   62.92%   64.82%   +1.89%     
==========================================
  Files         187      200      +13     
  Lines       12359    12542     +183     
==========================================
+ Hits         7777     8130     +353     
+ Misses       3897     3702     -195     
- Partials      685      710      +25

Impacted Files	Coverage Δ
hack/generator/pkg/astmodel/resource_type.go	`79.02% <0.00%> (+5.44%)`	⬆️
hack/generator/pkg/astmodel/type_walker.go	`67.53% <ø> (ø)`
hack/generator/pkg/astmodel/types.go	`59.37% <ø> (+2.08%)`	⬆️
...enerator/pkg/codegen/storage/property_converter.go	`57.81% <ø> (+57.81%)`	⬆️
...g/testcases/object_type_serialization_test_case.go	`2.06% <0.00%> (-0.04%)`	⬇️
hack/generator/pkg/astmodel/errored_type.go	`33.33% <50.00%> (ø)`
...n/pipeline/inject_property_assignment_functions.go	`66.66% <66.66%> (ø)`
...ck/generator/pkg/codegen/storage/type_converter.go	`69.81% <66.66%> (+69.81%)`	⬆️
hack/generator/pkg/astmodel/function_container.go	`71.42% <71.42%> (ø)`
hack/generator/pkg/astmodel/property_container.go	`71.42% <71.42%> (ø)`
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e458b2...393d2a7. Read the comment docs.

matthchr

Not done yet but posting what I have

hack/generator/pkg/astbuilder/composite_literals.go

hack/generator/pkg/astmodel/object_type.go

hack/generator/pkg/codegen/pipeline/create_storage_types.go

hack/generator/pkg/codegen/storage/storage_type_factory.go

...en/storage/testdata/Test_StorageTypeFactory_GeneratesExpectedResults-v20211231storage.golden

hack/generator/pkg/codegen/storage/type_converter.go

hack/generator/pkg/functions/original_gvk_function.go

hack/generator/pkg/functions/original_version_function.go

hack/generator/pkg/functions/original_version_function_test.go

hack/generator/pkg/codegen/storage/storage_type_factory.go

hack/generator/pkg/codegen/pipeline/create_storage_types.go

babbageclunk

This looks great, although I think the change you discussed of breaking StorageTypeFactory into a set of pipeline stages will be nicer.

hack/generator/pkg/astmodel/object_type.go

hack/generator/pkg/codegen/storage/storage_type_factory.go

...en/storage/testdata/Test_StorageTypeFactory_GeneratesExpectedResults-v20211231storage.golden

hack/generator/pkg/codegen/storage/type_converter.go

I've majorly restructured the PR, so this approval is stale.

matthchr

LGTM - Left a bunch of nits and one comment about golden test file structure.

hack/generator/pkg/astmodel/function_container.go

hack/generator/pkg/astmodel/property_container.go

matthchr · 2021-07-06T21:35:49Z

hack/generator/pkg/codegen/code_generator.go

@@ -151,7 +156,14 @@ func createAllPipelineStages(idFactory astmodel.IdentifierFactory, configuration
 		pipeline.AddCrossplaneEmbeddedResourceSpec(idFactory).UsedFor(pipeline.CrossplaneTarget),
 		pipeline.AddCrossplaneEmbeddedResourceStatus(idFactory).UsedFor(pipeline.CrossplaneTarget),

-		pipeline.CreateStorageTypes(idFactory).UsedFor(pipeline.ARMTarget), // TODO: For now only used for ARM
+		// Create Storage types
+		//TODO: For now only used for ARM


extra mega minor:

Suggested change

//TODO: For now only used for ARM

// TODO: For now only used for ARM

hack/generator/pkg/codegen/pipeline/create_storage_types.go

.../pkg/codegen/pipeline/testdata/TestInjectPropertyAssignmentFunctions-v20211231storage.golden

hack/generator/pkg/codegen/storage/property_injector.go

hack/generator/pkg/codegen/storage/types_tools.go

matthchr · 2021-07-06T23:03:46Z

hack/generator/pkg/functions/original_gvk_function.go

+type OriginalVersionKind string
+
+const (
+	ReadProperty = OriginalVersionKind("property")


I hate that go doesn't have first class enum support, and the solution of consts kinda works but if the consts don't have the enum name in them it's sometimes hard to pick out which values belong to which type.

I know in some places we do something like OriginalVersionKindReadProperty (which is sorta how you'd do it in C, I think?), but we don't have that consistently and it is awkwardly long.

Does this bug other folks too? What do people prefer?

I used very simple names here to optimize the API when consuming; adding a unique suffix would make that clunky.

Having first class enum support would be preferable - but I guess Go is designed to push people towards many simpler packages.

hack/generator/pkg/test/asserts.go

…rate types

theunrepentantgeek requested review from babbageclunk, davefellows, matthchr and Porges as code owners June 26, 2021 01:43

theunrepentantgeek self-assigned this Jun 26, 2021

theunrepentantgeek added this to the codegen-alpha-1 milestone Jun 26, 2021

theunrepentantgeek changed the title ~~Feature/persist apiversions~~ Persist original ARM API version for use by generic controller Jun 26, 2021

theunrepentantgeek force-pushed the feature/persist-apiversions branch from 5e16ac9 to 3f62792 Compare June 27, 2021 00:13

matthchr reviewed Jun 29, 2021

View reviewed changes

matthchr previously approved these changes Jun 29, 2021

View reviewed changes

babbageclunk previously approved these changes Jun 29, 2021

View reviewed changes

theunrepentantgeek force-pushed the feature/persist-apiversions branch from f36dd42 to d2da249 Compare June 30, 2021 08:16

theunrepentantgeek mentioned this pull request Jul 1, 2021

Inject Hub() function on hub storage resources #1616

Merged

1 task

theunrepentantgeek force-pushed the feature/persist-apiversions branch 2 times, most recently from 746553c to 0bc3f8a Compare July 5, 2021 05:39

This was referenced Jul 5, 2021

Convert Direction from enum into a discriminated union #1618

Merged

Move Property Assignment Function into the functions package #1622

Merged

babbageclunk approved these changes Jul 6, 2021

View reviewed changes

matthchr approved these changes Jul 6, 2021

View reviewed changes

theunrepentantgeek added 7 commits July 7, 2021 13:41

Add error for pipeline stage occurring too early

f6c315e

Create OriginalGVK() function

22fc434

Code gardening

1403056

Inject OriginalGVK functions and properties

9f663fd

Add OverlayWith() function for creating new Types sets

2cd02fe

Include functions when collecting referenced types

7d85225

Fix test case generation for new types

7724961

theunrepentantgeek added 25 commits July 7, 2021 13:41

Move creation of StorageTypes into dedicated pipeline stage

67968a4

Move AsFunctionContainer() into astmodel

03fba1b

Move AsPropertyContainer into astmodel

25f00a0

Move InjectOriginalGVKFunction into dedicated pipeline stage

c50f65c

Extract common building blocks for tests

9e3c5ac

Move InjectPropertyAssignmentFunctions to dedicated pipeline stage

62b2eb5

Set up stage prerequisites

fbdf89e

Simplify storage type conversion

4c7bdbf

Integrate new stages into pipeline

98a323e

Add pipeline stage InjectOriginalVersion property

953f26a

Update golden files

b3a4829

Fix Function receivers

46fe7d1

Add missing headers

571c43d

Fix tests

02dc3f6

Code gardening

b18c88d

Code gardening

2ddebc4

go fmt

ed07fb3

Filter out ARM types

e7bf505

Fix bug causing type conversion to be skipped

6ecf285

Empty commit

2a9cb67

Code gardening

74631c5

Use intention revealing names for predicates

4b4b717

Remove unused idFactory

f0eb8c5

Extract filter into predicate lambda local variable

59d8dbc

Make it explicit that we inject OriginalVersion() before creating sto…

3be33df

…rate types

theunrepentantgeek force-pushed the feature/persist-apiversions branch from 74e3f6f to 3be33df Compare July 7, 2021 01:42

Fixes for CI

393d2a7

theunrepentantgeek merged commit 2e113ed into master Jul 7, 2021

theunrepentantgeek deleted the feature/persist-apiversions branch July 7, 2021 01:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist original ARM API version for use by generic controller #1606

Persist original ARM API version for use by generic controller #1606

theunrepentantgeek commented Jun 26, 2021 •

edited

Loading

codecov-commenter commented Jun 27, 2021 •

edited

Loading

matthchr left a comment

babbageclunk left a comment

matthchr left a comment

matthchr Jul 6, 2021

matthchr Jul 6, 2021

theunrepentantgeek Jul 7, 2021

	//TODO: For now only used for ARM
	// TODO: For now only used for ARM

Persist original ARM API version for use by generic controller #1606

Persist original ARM API version for use by generic controller #1606

Conversation

theunrepentantgeek commented Jun 26, 2021 • edited Loading

codecov-commenter commented Jun 27, 2021 • edited Loading

Codecov Report

matthchr left a comment

Choose a reason for hiding this comment

babbageclunk left a comment

Choose a reason for hiding this comment

matthchr left a comment

Choose a reason for hiding this comment

matthchr Jul 6, 2021

Choose a reason for hiding this comment

matthchr Jul 6, 2021

Choose a reason for hiding this comment

theunrepentantgeek Jul 7, 2021

Choose a reason for hiding this comment

theunrepentantgeek commented Jun 26, 2021 •

edited

Loading

codecov-commenter commented Jun 27, 2021 •

edited

Loading